[HLSL] Fix resource wrapper declaration #125718

s-perron · 2025-02-04T16:53:11Z

The resource wrapper should have internal linkage because it contains a
handle to the global resource, and it not the actual global.

Making this changed exposed that we were zeroinitializing the resource,
which is a problem. The handle cannot be zeroinitialized.

Fixes #122767.

llvmbot · 2025-02-04T16:54:07Z

@llvm/pr-subscribers-hlsl
@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Steven Perron (s-perron)

Changes

The resource wrapper should have internal linkage because it cotains a
handle to the global resource, and it not the actual global.

Makeing this changed exposed that we were zeroinitializing the resouce,
which is a problem. The handle cannot be zeroinitialized.

Fixes #122767.

Full diff: https://github.com/llvm/llvm-project/pull/125718.diff

7 Files Affected:

(modified) clang/lib/CodeGen/CGHLSLRuntime.cpp (+6-6)
(modified) clang/lib/CodeGen/CGHLSLRuntime.h (+2)
(modified) clang/lib/CodeGen/CodeGenModule.cpp (+9-1)
(modified) clang/test/CodeGenHLSL/builtins/ByteAddressBuffers-constructors.hlsl (+3-3)
(modified) clang/test/CodeGenHLSL/builtins/RWBuffer-constructor-opt.hlsl (+3-11)
(modified) clang/test/CodeGenHLSL/builtins/RWBuffer-constructor.hlsl (+1-1)
(modified) clang/test/CodeGenHLSL/builtins/StructuredBuffers-constructors.hlsl (+5-5)

diff --git a/clang/lib/CodeGen/CGHLSLRuntime.cpp b/clang/lib/CodeGen/CGHLSLRuntime.cpp
index 2ce54cc3c52efa..955e32eb19fccc 100644
--- a/clang/lib/CodeGen/CGHLSLRuntime.cpp
+++ b/clang/lib/CodeGen/CGHLSLRuntime.cpp
@@ -521,11 +521,6 @@ void CGHLSLRuntime::generateGlobalCtorDtorCalls() {
   }
 }
 
-// Returns true if the type is an HLSL resource class
-static bool isResourceRecordType(const clang::Type *Ty) {
-  return HLSLAttributedResourceType::findHandleTypeOnResource(Ty) != nullptr;
-}
-
 static void createResourceInitFn(CodeGenModule &CGM, const VarDecl *VD,
                                  llvm::GlobalVariable *GV, unsigned Slot,
                                  unsigned Space) {
@@ -592,7 +587,7 @@ void CGHLSLRuntime::handleGlobalVarDefinition(const VarDecl *VD,
     // on?
     return;
 
-  if (!isResourceRecordType(VD->getType().getTypePtr()))
+  if (!isResource(VD))
     // FIXME: Only simple declarations of resources are supported for now.
     // Arrays of resources or resources in user defined classes are
     // not implemented yet.
@@ -616,3 +611,8 @@ llvm::Instruction *CGHLSLRuntime::getConvergenceToken(BasicBlock &BB) {
   llvm_unreachable("Convergence token should have been emitted.");
   return nullptr;
 }
+
+bool CGHLSLRuntime::isResource(const VarDecl *D) {
+  const clang::Type *ty = D->getType().getTypePtr();
+  return HLSLAttributedResourceType::findHandleTypeOnResource(ty) != nullptr;
+}
diff --git a/clang/lib/CodeGen/CGHLSLRuntime.h b/clang/lib/CodeGen/CGHLSLRuntime.h
index 032b2dee82f211..1c375837b4bb29 100644
--- a/clang/lib/CodeGen/CGHLSLRuntime.h
+++ b/clang/lib/CodeGen/CGHLSLRuntime.h
@@ -135,6 +135,8 @@ class CGHLSLRuntime {
     llvm::StructType *LayoutStruct = nullptr;
   };
 
+  bool isResource(const VarDecl *D);
+
 protected:
   CodeGenModule &CGM;
 
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 82002b8d8e4d4f..f24d733e13fedb 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -5584,7 +5584,10 @@ void CodeGenModule::EmitGlobalVarDefinition(const VarDecl *D,
       if (D->getType()->isReferenceType())
         T = D->getType();
 
-      if (getLangOpts().CPlusPlus) {
+      if (getLangOpts().HLSL && getHLSLRuntime().isResource(D)) {
+        Init = llvm::UndefValue::get(getTypes().ConvertType(ASTTy));
+        NeedsGlobalCtor = true;
+      } else if (getLangOpts().CPlusPlus) {
         Init = EmitNullConstant(T);
         if (!IsDefinitionAvailableExternally)
           NeedsGlobalCtor = true;
@@ -5730,6 +5733,11 @@ void CodeGenModule::EmitGlobalVarDefinition(const VarDecl *D,
       !D->hasAttr<ConstInitAttr>())
     Linkage = llvm::GlobalValue::InternalLinkage;
 
+  // For HLSL resources, the GV is an internal wrapper containing a handle to
+  // external resource.
+  if (getLangOpts().HLSL && getHLSLRuntime().isResource(D))
+    Linkage = llvm::GlobalValue::InternalLinkage;
+
   GV->setLinkage(Linkage);
   if (D->hasAttr<DLLImportAttr>())
     GV->setDLLStorageClass(llvm::GlobalVariable::DLLImportStorageClass);
diff --git a/clang/test/CodeGenHLSL/builtins/ByteAddressBuffers-constructors.hlsl b/clang/test/CodeGenHLSL/builtins/ByteAddressBuffers-constructors.hlsl
index 7fc6f4bb05745b..0e1ef3c70e7dd2 100644
--- a/clang/test/CodeGenHLSL/builtins/ByteAddressBuffers-constructors.hlsl
+++ b/clang/test/CodeGenHLSL/builtins/ByteAddressBuffers-constructors.hlsl
@@ -11,9 +11,9 @@ RasterizerOrderedByteAddressBuffer Buffer2: register(u3, space4);
 // CHECK: "class.hlsl::RWByteAddressBuffer" = type { target("dx.RawBuffer", i8, 1, 0) }
 // CHECK: "class.hlsl::RasterizerOrderedByteAddressBuffer" = type { target("dx.RawBuffer", i8, 1, 1) }
 
-// CHECK: @Buffer0 = global %"class.hlsl::ByteAddressBuffer" zeroinitializer, align 4
-// CHECK: @Buffer1 = global %"class.hlsl::RWByteAddressBuffer" zeroinitializer, align 4
-// CHECK: @Buffer2 = global %"class.hlsl::RasterizerOrderedByteAddressBuffer" zeroinitializer, align 4
+// CHECK: @Buffer0 = internal global %"class.hlsl::ByteAddressBuffer" undef, align 4
+// CHECK: @Buffer1 = internal global %"class.hlsl::RWByteAddressBuffer" undef, align 4
+// CHECK: @Buffer2 = internal global %"class.hlsl::RasterizerOrderedByteAddressBuffer" undef, align 4
 
 // CHECK; define internal void @_init_resource_Buffer0()
 // CHECK-DXIL: %Buffer0_h = call target("dx.RawBuffer", i8, 0, 0) @llvm.dx.resource.handlefrombinding.tdx.RawBuffer_i8_0_0t(i32 0, i32 0, i32 1, i32 0, i1 false)
diff --git a/clang/test/CodeGenHLSL/builtins/RWBuffer-constructor-opt.hlsl b/clang/test/CodeGenHLSL/builtins/RWBuffer-constructor-opt.hlsl
index 03f22620a097d9..239fb6d5569996 100644
--- a/clang/test/CodeGenHLSL/builtins/RWBuffer-constructor-opt.hlsl
+++ b/clang/test/CodeGenHLSL/builtins/RWBuffer-constructor-opt.hlsl
@@ -1,8 +1,7 @@
-// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -emit-llvm -O3 -o - %s | FileCheck %s --check-prefixes=CHECK,CHECK-DXIL
-// RUN: %clang_cc1 -triple spirv-vulkan-compute -x hlsl -emit-llvm -O3 -o - %s | FileCheck %s --check-prefixes=CHECK,CHECK-SPIRV
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -emit-llvm -O3 -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple spirv-vulkan-compute -x hlsl -emit-llvm -O3 -o - %s | FileCheck %s
 
-// CHECK-SPIRV: %"class.hlsl::RWBuffer" = type { target("spirv.Image", float, 5, 2, 0, 0, 2, 0) }
-// CHECK-DXIL:  %"class.hlsl::RWBuffer" = type { target("dx.TypedBuffer", float, 1, 0, 0) }
+// All referenced to an unused resource should be removed by optimizations.
 RWBuffer<float> Buf : register(u5, space3);
 
 [shader("compute")]
@@ -10,12 +9,5 @@ RWBuffer<float> Buf : register(u5, space3);
 void main() {
 // CHECK: define void @main()
 // CHECK-NEXT: entry:
-
-// CHECK-SPIRV-NEXT: %[[HANDLE:.*]] = tail call target("spirv.Image", float, 5, 2, 0, 0, 2, 0) @llvm.spv.resource.handlefrombinding.tspirv.Image_f32_5_2_0_0_2_0t(i32 3, i32 5, i32 1, i32 0, i1 false)
-// CHECK-SPIRV-NEXT: store target("spirv.Image", float, 5, 2, 0, 0, 2, 0) %[[HANDLE:.*]], ptr @Buf, align 8
-
-// CHECK-DXIL-NEXT: %[[HANDLE:.*]] = tail call target("dx.TypedBuffer", float, 1, 0, 0) @llvm.dx.resource.handlefrombinding.tdx.TypedBuffer_f32_1_0_0t(i32 3, i32 5, i32 1, i32 0, i1 false)
-// CHECK-DXIL-NEXT: store target("dx.TypedBuffer", float, 1, 0, 0) %[[HANDLE]], ptr @Buf, align 4
-
 // CHECK-NEXT: ret void
 }
diff --git a/clang/test/CodeGenHLSL/builtins/RWBuffer-constructor.hlsl b/clang/test/CodeGenHLSL/builtins/RWBuffer-constructor.hlsl
index d7cc3892a404bb..9527a8125fc564 100644
--- a/clang/test/CodeGenHLSL/builtins/RWBuffer-constructor.hlsl
+++ b/clang/test/CodeGenHLSL/builtins/RWBuffer-constructor.hlsl
@@ -7,7 +7,7 @@
 RWBuffer<float> Buf : register(u5, space3);
 
 // CHECK: %"class.hlsl::RWBuffer" = type { target("dx.TypedBuffer", float, 1, 0, 0) }
-// CHECK: @Buf = global %"class.hlsl::RWBuffer" zeroinitializer, align 4
+// CHECK: @Buf = internal global %"class.hlsl::RWBuffer" undef, align 4
 
 // CHECK: define internal void @_init_resource_Buf()
 // CHECK-DXIL: %Buf_h = call target("dx.TypedBuffer", float, 1, 0, 0) @llvm.dx.resource.handlefrombinding.tdx.TypedBuffer_f32_1_0_0t(i32 3, i32 5, i32 1, i32 0, i1 false)
diff --git a/clang/test/CodeGenHLSL/builtins/StructuredBuffers-constructors.hlsl b/clang/test/CodeGenHLSL/builtins/StructuredBuffers-constructors.hlsl
index bd931181045ba5..6a6a465092a620 100644
--- a/clang/test/CodeGenHLSL/builtins/StructuredBuffers-constructors.hlsl
+++ b/clang/test/CodeGenHLSL/builtins/StructuredBuffers-constructors.hlsl
@@ -15,11 +15,11 @@ RasterizerOrderedStructuredBuffer<float> Buf5 : register(u1, space2);
 // CHECK: %"class.hlsl::ConsumeStructuredBuffer" = type { target("dx.RawBuffer", float, 1, 0) }
 // CHECK: %"class.hlsl::RasterizerOrderedStructuredBuffer" = type { target("dx.RawBuffer", float, 1, 1) }
 
-// CHECK: @Buf = global %"class.hlsl::StructuredBuffer" zeroinitializer, align 4
-// CHECK: @Buf2 = global %"class.hlsl::RWStructuredBuffer" zeroinitializer, align 4
-// CHECK: @Buf3 = global %"class.hlsl::AppendStructuredBuffer" zeroinitializer, align 4
-// CHECK: @Buf4 = global %"class.hlsl::ConsumeStructuredBuffer" zeroinitializer, align 4
-// CHECK: @Buf5 = global %"class.hlsl::RasterizerOrderedStructuredBuffer" zeroinitializer, align 4
+// CHECK: @Buf = internal global %"class.hlsl::StructuredBuffer" undef, align 4
+// CHECK: @Buf2 = internal global %"class.hlsl::RWStructuredBuffer" undef, align 4
+// CHECK: @Buf3 = internal global %"class.hlsl::AppendStructuredBuffer" undef, align 4
+// CHECK: @Buf4 = internal global %"class.hlsl::ConsumeStructuredBuffer" undef, align 4
+// CHECK: @Buf5 = internal global %"class.hlsl::RasterizerOrderedStructuredBuffer" undef, align 4
 
 // CHECK: define internal void @_init_resource_Buf()
 // CHECK-DXIL: %Buf_h = call target("dx.RawBuffer", float, 0, 0) @llvm.dx.resource.handlefrombinding.tdx.RawBuffer_f32_0_0t(i32 0, i32 10, i32 1, i32 0, i1 false)

github-actions · 2025-02-04T17:01:08Z

✅ With the latest revision this PR passed the undef deprecator.

llvm-beanz

I have a couple concerns about this approach. I'm curious for @bogner's thoughts since he has approved the PR.

If/when we support separate compilation, the user-facing exported "resource" object will be the resource wrapper, not the handle, so that indicates to me we should probably have the wrappers being public.
It feels wrong to do this in CodeGen. It seems like we should be identifying an appropriate storage class in Sema such that we assign the correct linkage automatically.

@hekota might also have some good insights here because she is dealing with some of HLSL's implicit storage class/address space nonsense in her work with cbuffers.

s-perron · 2025-02-11T16:55:45Z

The other option is #123811. However, can you clarify "separate compilation". It is not clear to me exactly what type of linking you have in mind. This make it hard for me to reason about what should be done, and when.

Are you thinking of linking multiple llvm-ir modules or SPIR-V modules? Are there calls to functions in other modules or does each module have it owns self contained entry points?

bogner · 2025-02-14T21:47:23Z

If/when we support separate compilation, the user-facing exported "resource" object will be the resource wrapper, not the handle, so that indicates to me we should probably have the wrappers being public.

Depending on which aspect of separate compilation you mean, there are a couple of things to note here:

For linking DXIL modules (ie, the "lib" profile), these wrappers are irrelevant - we'll create the appropriate globals in lowering
For linking LLVM IR modules containing HLSL programs, we probably don't want to make these unequivocally public or internal - a resource that's exported should be public, sure, but that IMO should be opt-in, not opt-out. The fact that these are always public today whether or not they need to be causes issues in both the SPIR-V and DirectX backends. In this case, presumably the linking phase would internalize the globals after linking much like LTO does in C/C++ workflows.

It feels wrong to do this in CodeGen. It seems like we should be identifying an appropriate storage class in Sema such that we assign the correct linkage automatically.

I'll defer to you on this point, but that does sound pretty reasonable.

The resource wrapper should have internal linkage because it cotains a handle to the global resource, and it not the actual global. Makeing this changed exposed that we were zeroinitializing the resouce, which is a problem. The handle cannot be zeroinitialized. Fixes llvm#122767.

s-perron · 2025-02-18T19:47:57Z

2. It feels wrong to do this in CodeGen. It seems like we should be identifying an appropriate storage class in Sema such that we assign the correct linkage automatically.

If we change the storage class in Sema, then codegen is not able to distinguish between:

static RWBuffer<int4> s;
RWBuffer<int4> global;

This causes problems during codegen because the resource init function is created only for the "global" variables, and not static. We could probably refactor that to make it work. Just be aware that it might be a bigger change than you might expect.

s-perron · 2025-02-26T19:17:49Z

We discussed, and we will be make the wrapper static, but it should be done in sema. I'll open a new PR for that change.

s-perron requested review from bogner and llvm-beanz February 4, 2025 16:53

llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. HLSL HLSL Language Support labels Feb 4, 2025

s-perron changed the title ~~[HLSL] Fix resrouce wrapper declaration~~ [HLSL] Fix resource wrapper declaration Feb 5, 2025

bogner approved these changes Feb 5, 2025

View reviewed changes

llvm-beanz reviewed Feb 11, 2025

View reviewed changes

s-perron force-pushed the static_wrapper branch from 085828e to a6ecf11 Compare February 18, 2025 15:31

s-perron added 2 commits February 18, 2025 13:25

Use poison instead of undef.

bcc685e

s-perron force-pushed the static_wrapper branch from a6ecf11 to bcc685e Compare February 18, 2025 18:25

s-perron marked this pull request as draft February 18, 2025 18:26

s-perron closed this Feb 26, 2025

github-project-automation bot moved this to Closed in HLSL Support Feb 26, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[HLSL] Fix resource wrapper declaration #125718

[HLSL] Fix resource wrapper declaration #125718

Uh oh!

s-perron commented Feb 4, 2025 •

edited

Loading

Uh oh!

llvmbot commented Feb 4, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Feb 4, 2025 •

edited

Loading

Uh oh!

llvm-beanz left a comment

Uh oh!

s-perron commented Feb 11, 2025

Uh oh!

bogner commented Feb 14, 2025

Uh oh!

s-perron commented Feb 18, 2025

Uh oh!

s-perron commented Feb 26, 2025 •

edited

Loading

Uh oh!

Uh oh!

[HLSL] Fix resource wrapper declaration #125718

[HLSL] Fix resource wrapper declaration #125718

Uh oh!

Conversation

s-perron commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvm-beanz left a comment

Choose a reason for hiding this comment

Uh oh!

s-perron commented Feb 11, 2025

Uh oh!

bogner commented Feb 14, 2025

Uh oh!

s-perron commented Feb 18, 2025

Uh oh!

s-perron commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

s-perron commented Feb 4, 2025 •

edited

Loading

llvmbot commented Feb 4, 2025 •

edited

Loading

github-actions bot commented Feb 4, 2025 •

edited

Loading

s-perron commented Feb 26, 2025 •

edited

Loading